Implement P256 verification via RIP-7212 precompile with Solidity fallback #4881

Amxx · 2024-02-07T10:25:52Z

Fixes LIB-1225

Why do we care about secp256r1?

Most security application uses secp256r1 (also known as p256). This lead hardware manufacturers to implement it, and leave other “exotic” curves on the side. Today, billions of people around the world own devices with spetial security hardware that supports secp256r1. If that was the curve used by ethereum, all these people would basically already own a hardware wallet … but unfortunatelly that is not the case.

If we cannot easily modify the curves supported by major smartphones manufacturer, we can provide tools to verify secp256r1 curve onchain. This would allow control of ERC-4337 smart wallets (among others) through a device designed to handle security keys (something users are notoriously bad at).

What @openzeppelin/contracts could provide

Existing wallets provide mechanisms to produce secp256k1 signature, both for transactions and messages. Solidity provides a precompile that, given a hash and a signature, will recover the address of the signer (using secp256k1). No such precompile exist for secp256r1.

There exist solidity implementations of the secp256r1 “verification” workflow. There is also a proposal to provide that verification through a precompile. Even if the precompile is implemented, it is likelly that many chains will not upgrade soon. A solidity implementation would remain usefull for users on these chains.

In some cases, users may want to follow the “recovery” flow that they are familiar with. There is also no proposal for a precompile that would do that operation. A solidity implementation would possibly be usefull to many users, and remain uncontested in the near future.

Notes

Stack too depth

Current proposed implementation works well when turning optimization on. However, compilation fails with "stack to deep" if optimizations are NOT turned on. This PR does enable optimizations for all tests to circumvent this issue. Also, users will have to enable optimizations if they want to use this library, which they should definitelly do given the gast costs.

This is still an issue when running coverage :/
- This was fixed by adding details: { yul: true }, to the optimizer settings. This change in optimization setup may affect the accuracy of gas reporting in this PR (reference doesn't use the same settings)

Benchmarking

This repo provides benchmarking of this implementation against other existing ones.

PR Checklist

Tests
Documentation
Changeset entry (run npx changeset add)

changeset-bot · 2024-02-07T10:25:55Z

🦋 Changeset detected

Latest commit: 5314727

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

ernestognw

Left a couple of questions and notes while reviewing. I've been delaying this review for a while but at first sight it looks really well implemented, and I mostly need to familiarize and check the math is correct.

contracts/utils/cryptography/P256.sol

Co-authored-by: Ernesto García <ernestognw@gmail.com>

Amxx · 2024-04-25T13:34:15Z

Relevant source for discussion: https://www.hyperelliptic.org/EFD/g1p/auto-shortw-jacobian.html

contracts/utils/cryptography/P256.sol

Amxx · 2024-04-25T15:25:48Z

contracts/utils/cryptography/P256.sol

+                if (pos > 0) {
+                    (x, y, z) = _jAdd(x, y, z, points[pos].x, points[pos].y, points[pos].z);
+                }


So here the if is optional.

If we remove the if, we are going to load points[0] which is (0,0,0) ... and the _jAdd will skip that as the "neutral element". The if here as a cost. 15/16 we pay it for no real reason (and we still pay the check in _jAdd). 1/16 the if avoids the overhead of a function call.

I'm going to benchmark which one is better and comment that so we don't go back and forward.

I checked, skipping the mloads in 1/16 cases is a bigger gain than the loss of the if in the other 15/16 cases. Keeping the if is the more effective solution here

contracts/utils/cryptography/P256.sol

docs/modules/ROOT/pages/utilities.adoc

Co-authored-by: sudo rm -rf --no-preserve-root / <pcaversaccio@users.noreply.github.com>

hardhat.config.js

frangio · 2024-06-25T14:54:28Z

Re: Transaction reverted: trying to deploy a contract whose code is too large

I think allowUnlimitedContractSize can be safely set to true always.

If a non-exposed contract is too large it should trigger the compiler warning and fail CI anyway:

openzeppelin-contracts/hardhat.config.js

Lines 91 to 97 in c3f8b76

    
           warnings: { 
        
             'contracts-exposed/**/*': { 
        
               'code-size': 'off', 
        
               'initcode-size': 'off', 
        
             }, 
        
             '*': { 
        
               'code-size': withOptimizations,

ernestognw · 2024-06-25T15:00:49Z

I understand the logic for 9b24014, but honestly that is an abomination. We surelly can do better.

I'd like to hear why this is a problem if using scratch space is safe. Regardless of how hacky it is I do think it's worse that we didn't discuss the config and is again there. I would appreciate if you communicate better your decisions. It's pretty much impossible to follow your preferences here!

Moved that part to a separate PR OpenZeppelin#5099

contracts/utils/cryptography/P256.sol

cairoeth

nit

contracts/utils/cryptography/P256.sol

ernestognw · 2024-07-03T06:26:36Z

It turns out that after #5098, forge coverage hits the Stack too deep error again although tests compile without --via-ir.
We can use --ir-minimum in scripts/checks/coverage.sh and should work. I'll try it out. See foundry-rs/foundry#3357

ernestognw

LGTM, as I pointed out, I think it's fine that forge coverage runs with --ir-minimum

Amxx added 4 commits February 7, 2024 10:49

Add P256 implementation and testing

5e82076

enable optimizations by default

da0f27e

test recovering address

aa59c67

improved testing

9512947

Amxx added the Cryptography label Feb 7, 2024

Amxx added this to the 5.1 milestone Feb 7, 2024

Amxx added 12 commits February 7, 2024 11:38

spelling

a60bf48

fix lint

9185026

expose imports tick

025e360

fix lint

803e735

fix lint

57fcecd

add changeset

4dae298

improve doc

6cf039d

add envvar to force allowUnlimitedContractSize

c094fa1

fix lint

20a03df

fix stack too deep error in coverage

15f1a6b

reoder arguments to match ecrecover and EIP-7212

e2040e4

reduce diff

695b732

Amxx marked this pull request as ready for review March 13, 2024 14:01

Merge branch 'master' into feature/P256

41aaf71

ernestognw reviewed Apr 23, 2024

View reviewed changes

Update contracts/utils/cryptography/P256.sol

3bf4557

Co-authored-by: Ernesto García <ernestognw@gmail.com>

Amxx added 2 commits April 25, 2024 15:35

Merge branch 'master' into feature/P256

3cbf426

update pseudocode reference

bba7fa3

Amxx commented Apr 25, 2024

View reviewed changes

contracts/utils/cryptography/P256.sol Outdated Show resolved Hide resolved

Update contracts/utils/cryptography/P256.sol

2812ed8

Amxx commented Apr 25, 2024

View reviewed changes

refactor neutral element in jAdd

e0ef63b

Amxx commented Jun 25, 2024

View reviewed changes

contracts/utils/cryptography/P256.sol Outdated Show resolved Hide resolved

Update contracts/utils/cryptography/P256.sol

fbc11f5

pcaversaccio reviewed Jun 25, 2024

View reviewed changes

docs/modules/ROOT/pages/utilities.adoc Outdated Show resolved Hide resolved

docs/modules/ROOT/pages/utilities.adoc Outdated Show resolved Hide resolved

Apply suggestions from code review

9c88101

Co-authored-by: sudo rm -rf --no-preserve-root / <pcaversaccio@users.noreply.github.com>

Amxx commented Jun 25, 2024

View reviewed changes

hardhat.config.js Outdated Show resolved Hide resolved

Update hardhat.config.js

b5e6bd7

Amxx added 4 commits June 25, 2024 17:43

Update hardhat.config.js

db76353

Update hardhat.config.js

0722d93

Revert all changes to hardhat.config.js

306a5f6

Moved that part to a separate PR OpenZeppelin#5099

uniform style

e67a456

cairoeth reviewed Jun 26, 2024

View reviewed changes

contracts/utils/cryptography/P256.sol Outdated Show resolved Hide resolved

Amxx added 2 commits June 26, 2024 17:01

add bound checks to isOnCurve

1a8cb63

rename isOnCurve -> isValidPublicKey + add _isProperSignature helper

3c3fa27

ernestognw reviewed Jun 27, 2024

View reviewed changes

contracts/utils/cryptography/P256.sol Outdated Show resolved Hide resolved

Update contracts/utils/cryptography/P256.sol

2fe4a16

ernestognw reviewed Jun 27, 2024

View reviewed changes

contracts/utils/cryptography/P256.sol Show resolved Hide resolved

cairoeth reviewed Jun 30, 2024

View reviewed changes

contracts/utils/cryptography/P256.sol Show resolved Hide resolved

ernestognw added 2 commits July 1, 2024 14:16

Update contracts/utils/cryptography/P256.sol

2420d13

Merge branch 'master' into feature/P256

49f3ad9

Enable --ir-minimum in forge coverage

5314727

ernestognw approved these changes Jul 3, 2024

View reviewed changes

Amxx merged commit 05f218f into OpenZeppelin:master Jul 3, 2024
21 checks passed

Amxx deleted the feature/P256 branch July 3, 2024 07:17

This was referenced Nov 8, 2024

[Snyk] Upgrade @openzeppelin/contracts from 5.0.0 to 5.1.0 RomulousApollo/v3-core#4

Open

[Snyk] Upgrade @openzeppelin/contracts-upgradeable from 5.0.0 to 5.1.0 RomulousApollo/v3-core#5

Open

This was referenced Nov 9, 2024

[Snyk] Upgrade @openzeppelin/contracts from 5.0.0 to 5.1.0 doperiddle/stl-contracts#5

Merged

[Snyk] Upgrade @openzeppelin/contracts-upgradeable from 5.0.0 to 5.1.0 doperiddle/stl-contracts#7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement P256 verification via RIP-7212 precompile with Solidity fallback #4881

Implement P256 verification via RIP-7212 precompile with Solidity fallback #4881

Amxx commented Feb 7, 2024 •

edited by ernestognw

Loading

changeset-bot bot commented Feb 7, 2024 •

edited

Loading

ernestognw left a comment

Amxx commented Apr 25, 2024

Amxx Apr 25, 2024

Amxx Apr 25, 2024

frangio commented Jun 25, 2024

ernestognw commented Jun 25, 2024

cairoeth left a comment

ernestognw commented Jul 3, 2024 •

edited

Loading

ernestognw left a comment

Implement P256 verification via RIP-7212 precompile with Solidity fallback #4881

Implement P256 verification via RIP-7212 precompile with Solidity fallback #4881

Conversation

Amxx commented Feb 7, 2024 • edited by ernestognw Loading

Why do we care about secp256r1?

What @openzeppelin/contracts could provide

Notes

Stack too depth

Benchmarking

PR Checklist

changeset-bot bot commented Feb 7, 2024 • edited Loading

🦋 Changeset detected

ernestognw left a comment

Choose a reason for hiding this comment

Amxx commented Apr 25, 2024

Amxx Apr 25, 2024

Choose a reason for hiding this comment

Amxx Apr 25, 2024

Choose a reason for hiding this comment

frangio commented Jun 25, 2024

ernestognw commented Jun 25, 2024

cairoeth left a comment

Choose a reason for hiding this comment

ernestognw commented Jul 3, 2024 • edited Loading

ernestognw left a comment

Choose a reason for hiding this comment

Amxx commented Feb 7, 2024 •

edited by ernestognw

Loading

changeset-bot bot commented Feb 7, 2024 •

edited

Loading

ernestognw commented Jul 3, 2024 •

edited

Loading